[Distributed] Fix tiktokenizer decoding #1257
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1257
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e54c8d1 with merge base 34d6831 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
| res = torch.cat(res, dim=1) | ||
| res_list = res.tolist() | ||
| responses = tokenizer.decode(res_list) | ||
| if isinstance(tokenizer, TiktokenTokenizer): |
There was a problem hiding this comment.
nit - I think this type of check is better done directly in the _build_chat_tokenizer function and then we just have an enum for the tokenizer type and can correctly err out if a not recognized tokenizer.
The reason for that is two fold:
a - we future proof ourself so that if llama4 has a new tokenizer we are not going back through the code trying to figure where all the 'tokenizer' type checks are, and updating.
b - we only check once in a logical point, and then all other code uses the enum, and we have an upfront single failure point to err out if we are hitting an unrecognized tokenizer.
As this code is currently, it assumes that if not tiktoken then it must be sentencepiece which is a brittle assumption long term.
|
Thanks @lessw2020 . Do you know why TiktokenTokenizer cannot decode 2D list? |
TiktokenTokenizer(used by llama3) andSentencePieceProcessor(used by llama2) seem to have different requirement on input shape (1D list or 2D list).Adding a condition to separate the input prep for the two.
Also improved some logging.